refactor(api): read the recorded metric bundle path and stop dropping groups - #173

Nitpick comments: 3

backend/tests/test_api/test_routes/test_executions.py: 2

689-692: Trivial · Maintainability & Code Quality · Quick win

Test the recorded bundle path, not only diagnostic.json.

metric_bundle now resolves the bundle from the path recorded on the execution. The success test uses only fixture data that records diagnostic.json. A route that still reads the fixed diagnostic.json name would return the same file and pass. The change to the recorded path is not covered.

Add a fixture where an execution records a different bundle filename, with that file present in the expected output location. Assert that the endpoint returns that file's contents. Keep the existing 404 test for an execution with no recorded bundle.

---

696-697: Trivial · Maintainability & Code Quality · Quick win

Assert the missing-bundle detail, not only the 404 status.

The test test_metric_bundle_404_without_recorded_bundle checks only status_code == 404. The route returns 404 from two branches. A missing execution, or one that belongs to another group, returns Result not found. A recorded bundle file that is missing returns Metrics bundle not found. The status-only assertion cannot tell these apart. If the lookup regresses to the first branch, the test still passes. Asserting the detail text confirms that the request reaches the bundle branch.

Suggested fix

 def test_metric_bundle_404_without_recorded_bundle(client: TestClient, settings) -> None:
     r = client.get(f"{settings.API_V1_STR}/executions/37/metric_bundle?execution_id=20")
     assert r.status_code == 404
+    assert r.json()["detail"] == "Metrics bundle not found"

backend/src/ref_backend/api/routes/executions.py: 1

140-140: Trivial · Maintainability & Code Quality · Quick win

Add a regression test for a failed group build in GET /executions/.

The list route no longer catches a build failure. A failing group now fails the whole request. No test covers this path, so restoring the former catch-and-omit logic would still pass the current suite. The shared client fixture uses TestClient(app) with default exception handling, so the exception re-raises in the test instead of returning a response. The test must disable that behaviour and assert the 500 response.

🧪 Suggested test

+def test_execution_list_fails_when_a_group_cannot_be_built(app, settings, monkeypatch) -> None:
+    def fail_build(*args, **kwargs):
+        raise RuntimeError("build failed")
+
+    monkeypatch.setattr(executions_routes.ExecutionGroup, "build", fail_build)
+
+    with TestClient(app, raise_server_exceptions=False) as client:
+        r = client.get(f"{settings.API_V1_STR}/executions")
+
+    assert r.status_code == 500

Review info

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: ca98b268-e13a-45b7-b8f7-9a94fde81403

Commits

Reviewing files that changed from the base of the PR and between 7130f0f and 21fb1f1.

Files selected for processing: 5

• backend/src/ref_backend/api/routes/executions.py
• backend/src/ref_backend/core/file_handling.py
• backend/tests/test_api/test_routes/test_executions.py
• changelog/173.fix.md
• changelog/173.improvement.md

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

Nitpick comments: 2

backend/src/ref_backend/core/file_handling.py: 1

30-30: Trivial · Maintainability & Code Quality · Low value

Add a unit test that passes a keyword argument to resolve_artifact.

The endpoint tests do not cover the **kwargs forwarding. Five tests replace resolve_artifact with monkeypatch. The unmocked test_metric_bundle reaches the real helper, but only with positional arguments. No test passes a keyword argument. If a later change removes **kwargs, a keyword call fails with TypeError, and no test detects it.

The current forwarding is correct. Add a direct test of the real helper:

🧪 Suggested test

from pathlib import Path

from ref_backend.core.file_handling import resolve_artifact


def test_resolve_artifact_forwards_keyword_arguments() -> None:
    def resolve(*, fragment: str, filename: str) -> Path:
        return Path(fragment) / filename

    assert resolve_artifact(resolve, fragment="some", filename="custom.json") == Path("some/custom.json")

frontend/src/components/diagnostics/figureGalleryData.ts: 1

51-52: Trivial · Maintainability & Code Quality · Low value

Add a regression test for null description values.

filterFigures skips null description and filename values before it runs the pattern. No test in figureGalleryData.test.ts uses a null field. If the null check were removed, pattern.test(null) would convert null to "null". A search for null would then match the figure. The current tests would still pass.

Add a figure with a null description and a filename that does not contain null, then assert that the search excludes it:

🧪 Suggested test

   it("keeps the figures whose group matches the selector filters", () => {
     expect(
       filenames(filterFigures(figures, "", { source_id: ["MIROC6"] })),
     ).toEqual(["pr_bias.png"]);
     expect(filterFigures(figures, "tas", { source_id: ["MIROC6"] })).toEqual(
       [],
     );
   });
+
+  it("ignores null metadata when matching text", () => {
+    const nullDescription = collectLatestPlotFigures([
+      group([
+        [
+          {
+            id: nextId++,
+            filename: "lat_bias.png",
+            description: null,
+            output_type: "plot",
+          } as ExecutionOutput,
+        ],
+      ]),
+    ]);
+    expect(filterFigures(nullDescription, "null", {})).toEqual([]);
+  });
 });

The fixture uses an object literal because the output() helper does not accept null. The filename lat_bias.png does not match null, so the assertion depends only on the description guard.